Consolidate bounded pane context with GetPaneContext - #884
Consolidate bounded pane context with GetPaneContext#884Kai Tao (vanzue) wants to merge 6 commits into
Conversation
Add GetPaneContext protocol support, shared planner and Autofix collection, bounded capture coverage, and reproducible same-build performance benchmarks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🟡 Changes recommended
wtcli get-pane-context currently fails silently on an invalid --target GUID (non-zero exit without an error message), which should be fixed for debuggability and script reliability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements the consolidated, bounded pane-context operation for #838 by appending GetPaneContext to the existing Terminal Protocol COM interface (protocol 2.3), exposing it via wtcli get-pane-context, and routing WTA planner/manual-fix/autofix context collection through the single consolidated request with bounded capture and accurate truncation reporting.
Changes:
- Add protocol 2.3
GetPaneContextend-to-end (IDL/COM server/TerminalPage/ControlCore) with bounded marked-command capture and bounded buffer-tail fallback. - Add
wtcli get-pane-contextwith protocol/capability negotiation and max-lines/max-chars validation; add WTA channel support (get_pane_context) and migrate ACP prompt context collection to use it (with legacy fallback for unsupported servers). - Add E2E coverage for pane-context scenarios and a PowerShell benchmark harness + selftests + documentation + release checklist mapping.
File summaries
| File | Description |
|---|---|
| tools/wta/src/shell/wt_channel/cli_channel.rs | Map get_pane_context to a single wtcli get-pane-context invocation with bounds validation. |
| tools/wta/src/shell/shell_manager.rs | Add wt_get_pane_context helper routing through the WT channel. |
| tools/wta/src/protocol/acp/prompt_context.rs | Switch planner/autofix context capture to consolidated pane-context request; retain capability-gated legacy fallback. |
| tools/wta/src/protocol/acp/prompt_builder.rs | Remove direct shell manager field from ContextRequest and adapt tests/mocks to consolidated capture. |
| tools/wta/src/protocol/acp/mock_agent_tests.rs | Update blocking/mock WT channel behavior to serve consolidated pane-context payload shape. |
| test/e2e/tools/PaneContextBenchmark.ps1 | Add shared benchmark/statistics + UTF-16-surrogate-safe scalar counting and prompt truncation helpers. |
| test/e2e/tests/Feature.PaneContext.Tests.ps1 | Add packaged E2E tests covering marked/unmarked capture, isolation, missing panes, metadata-only, Unicode/truncation, and agent-focus resolution. |
| test/e2e/selftests/PaneContextBenchmark.Unit.Tests.ps1 | Add hermetic Pester selftests for benchmark math, Unicode handling, and request fidelity. |
| test/e2e/README.md | Document the new pane-context benchmark workflow and add the new PaneContext feature suite to the table. |
| test/e2e/Measure-PaneContext.ps1 | Add the benchmark driver script that validates stability and compares legacy vs consolidated collectors on the same deployed build. |
| src/tools/wtcli/main.cpp | Add get-pane-context subcommand with negotiation and validation; call ITerminalProtocol::GetPaneContext. |
| src/host/proxy/ITerminalProtocol.idl | Append COM GetPaneContext without reordering existing vtable slots (protocol 2.3). |
| src/cascadia/WindowsTerminal/TerminalProtocolComServer.h | Declare COM server implementation of GetPaneContext. |
| src/cascadia/WindowsTerminal/TerminalProtocolComServer.cpp | Advertise protocol 2.3 + capability; implement GetPaneContext routing to TerminalPage::GetProtocolPaneContext. |
| src/cascadia/ut_app/ProtocolParsingTests.cpp | Add unit tests for bounded UTF-8/line handling in command and buffer-tail builders. |
| src/cascadia/UnitTests_Control/ControlCoreTests.cpp | Add unit tests for ControlCore::ReadBufferTail behavior, bounds, and Unicode handling. |
| src/cascadia/TerminalProtocol/TerminalProtocol.idl | Add PaneContext struct and GetPaneContext WinRT method to the Terminal Protocol surface. |
| src/cascadia/TerminalProtocol/ProtocolParsing.h | Add bounded UTF-8-aware command and buffer-tail text builders used by pane-context capture. |
| src/cascadia/TerminalControl/TermControl.idl | Add ReadBufferTail and ReadLastPromptBounded to the TermControl API. |
| src/cascadia/TerminalControl/TermControl.h | Declare TermControl wrappers for new bounded read methods. |
| src/cascadia/TerminalControl/TermControl.cpp | Implement TermControl wrappers delegating to ControlCore. |
| src/cascadia/TerminalControl/ControlCore.idl | Add ReadBufferTail and ReadLastPromptBounded to ControlCore WinRT surface. |
| src/cascadia/TerminalControl/ControlCore.h | Declare new bounded read APIs on ControlCore. |
| src/cascadia/TerminalControl/ControlCore.cpp | Implement bounded buffer-tail reading and bounded mark-based last-prompt reading with Unicode-safe iteration. |
| src/cascadia/TerminalApp/TerminalPage.Protocol.cpp | Implement GetProtocolPaneContext resolving explicit/effective source pane and performing bounded capture/fallback. |
| src/cascadia/TerminalApp/TerminalPage.idl | Add GetProtocolPaneContext to TerminalPage IDL. |
| src/cascadia/TerminalApp/TerminalPage.h | Add GetProtocolPaneContext declaration. |
| doc/release-check-list.md | Add/reassign release checklist items C296–C303 for pane-context and routing coverage. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
Preserve consolidated context dispatch alongside upstream hook listener changes and reconcile release checklist IDs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🔵 Needs a closer look
It spans COM ABI changes, cross-process protocol negotiation, and UI-thread-adjacent context capture logic that merits final human validation.
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: add43dbc-523d-4150-ae65-bcda1ed72618
Move bounded text conversion off the UI thread while preserving apartment-safe references and fallback reads. Reject malformed, zero, and explicit empty targets before COM with one diagnostic. Split newline-adjacent literals for spelling and cover CLI diagnostics in packaged integration tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🔵 Needs a closer look
It spans COM ABI surface changes, new protocol semantics, and cross-language plumbing (C++/WinRT + CLI + Rust + E2E), which warrants final human review for compatibility and contract correctness.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/cascadia/TerminalApp/TerminalPage.Protocol.cpp:273
PaneInfo.Columnsis always reported as 0, which makes the exportedpane.size.columnsmetadata inaccurate forget-pane-contextresponses (and any consumers that rely on dimensions). If the column count is not available here, it would be better to either populate it correctly or omit the field from the JSON contract altogether.
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
Append a shared control-state width getter backed by the read-locked terminal viewport. Populate GetPaneContext dimensions without pixel estimates, and cover initial/resized widths plus packaged context metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
|
Addressed the suppressed Columns finding in review #884 (review) with commit 9397dba. GetPaneContext now reads the actual read-locked terminal viewport width through an appended ICoreState getter, rather than exporting zero or estimating from pixels. The projected ControlCore test verifies 30 columns initially and 50 after resizing. Fresh native builds and all seven packaged PaneContext cases passed, including positive dimensions in marked and metadata-only captures. Live metadata changed from zero columns before the fix to a valid 90-column viewport after deployment; user settings were preserved. |
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of concrete validation/privacy issues (typed session_id validation in WTA and overly-permissive ACP logging opt-in detection) that should be fixed before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tools/wta/src/logging.rs:60
explicitly_configures_acp_dependencyusesstarts_with("agent_client_protocol"), which will also treat typos likeagent_client_protocolx=debugas an explicit opt-in and thereby disable the privacy cap. Tighten the match to only accept the exact target or its=/::prefixes so prompt-body traces can’t be re-enabled accidentally.
tools/wta/src/shell/wt_channel/cli_channel.rs:975- The
get_pane_contexthandler claims'session_id' must be a string, but it usesjson_id_as_str, which also accepts JSON numbers. That means numeric IDs will slip through this validation and only fail later inwtclias an invalid GUID, making diagnostics inconsistent with the stated contract (pane session IDs are GUID strings).
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
Reject non-string pane session IDs before launching wtcli without changing numeric tab/window IDs. Require a complete ACP dependency target boundary so lookalike logging directives cannot remove the privacy cap. Cover both previously failing cases with regression assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
|
Addressed both suppressed findings from #884 (review) in 11df477. ACP logging opt-in now requires the exact dependency target or a valid = / :: boundary; lookalike targets retain the privacy cap, with emitted-log regression coverage. get_pane_context now strictly requires a string session_id and rejects invalid types before invoking wtcli, without changing numeric tab/window IDs elsewhere. Both regressions failed before the fix. Full WTA: 2086 passed, 1 ignored. After a WTA-only refresh of the existing Dev layout, packaged PaneContext + AutofixParser integration passed 11/11 with no skips; settings were preserved. |
There was a problem hiding this comment.
🔵 Needs a closer look
wtcli’s new capability negotiation currently conflates GetCapabilities/JSON failures with “unsupported” (exit code 2), masking real errors and making automation/fallback behavior unreliable.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/tools/wtcli/main.cpp:344
- SupportsCapability collapses GetCapabilities/JSON failures into a plain "false" result. In get-pane-context this can misreport a server/contract error as WT_PROTOCOL_UNSUPPORTED_PANE_CONTEXT (exit code 2) rather than surfacing the underlying failure (exit code 1), which makes troubleshooting and scripting unreliable.
This issue also appears on line 606 of the same file.
tools/wta/src/protocol/acp/prompt_context.rs:327
- capture_pane_context will silently drop a successful wtcli response if the JSON payload is missing the required top-level "pane" field (or if the response isn't an object), because value.get("pane")? early-returns None without emitting any diagnostic. That makes server/CLI contract regressions hard to detect in logs (it looks the same as a normal capture failure).
src/tools/wtcli/main.cpp:614
- get-pane-context treats any GetCapabilities failure (or malformed capabilities JSON) as "unsupported" and returns exit code 2, even though the failure may be a server/connection/contract error. This masks real errors behind WT_PROTOCOL_UNSUPPORTED_PANE_CONTEXT and makes fallback logic in callers less trustworthy.
if (!ProtocolAtLeast(version, 2, 3) ||
!SupportsCapability(server.get(), "get_pane_context"))
{
fprintf(stderr,
"[wtcli] WT_PROTOCOL_UNSUPPORTED_PANE_CONTEXT server=%s required=2.3\n",
version.empty() ? "unknown" : version.c_str());
exitCode = 2;
return;
}
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Implements the consolidated, bounded context operation for #838, named
GetPaneContextas agreed during implementation.GetPaneContextto the existing COM protocol without changing earlier vtable slots; advertise protocol 2.3 andget_pane_context.wtcli get-pane-context, with version/capability negotiation before calling the appended method and validation of targets and capture budgets./fix, and error-triggered Autofix context collection through WTA -> wtcli -> the existing COM server. No direct Rust COM client and no new COM server.The Autofix event-delivery and cross-tab routing fixes were isolated and already merged in #880. They are part of this branch's main baseline, not new fixes in this PR.
Debug benchmark results
Important: this is a same-build comparison of two context-collection paths, not an A/B comparison of two separately built branches.
Both paths ran against the same locally deployed x64 Debug package built from
dev/vanzue/get-context, based on main commitdb609f8061f81c2eb9a4bdaf3e0666392596bce4, with the feature changes applied:IntelligentTerminal_0.8.0.2_x64__rd9vj3e6a2mbr.src\cascadia\CascadiaPackage\bin\x64\Debug\AppX.wtcli.exeand the same running Terminal/server. Deployed WTA and WindowsTerminal hashes matched the build outputs.active-panecall, followed by window/tab/pane enumeration and output capture. There is no artificially added unsupported-capability probe.wtcli get-pane-contextprocess captures the context. Authentication and capability negotiation still happen within wtcli; one process does not mean only one raw COM call./fix/fix/fixIn these measurements, median context-collection time decreased by 47.9%-88.9%, saving approximately 80-685 ms per collection. The new path's p50 stayed at approximately 84-87 ms.
Interpretation and limits
Reproduction and artifacts
The PR includes
test\e2e\Measure-PaneContext.ps1, its shared helpers, 16 hermetic selftests, and usage documentation intest\e2e\README.md.Example, with an already-running Dev package and a stable existing pane:
For Planner or ManualFix, focus the expected source pane first; the script does not change focus, send input, change settings, or start/stop Terminal. Each run exports
samples.csv,requests.csv,metadata.json, andsummary.json, including binary hashes, source provenance, and timing definitions.Local measurements are retained under
test\e2e\artifacts\pane-context-benchmark\, with the combined table indebug-comparison.csv. These ignored local artifacts are not committed or uploaded; the measured summary is reproduced above.Coverage and validation
7c3bdf7c2: 2,085 passed, 1 ignored. The following review-fix commit does not change Rust.Remaining validation
check-spellingcheck.